Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WiP: resolved some minor errors that raised from the initial clone of the branch #45

Draft
wants to merge 33 commits into
base: feature/openacc-library-routines
Choose a base branch
from

Conversation

monoatamd
Copy link

@monoatamd monoatamd commented Nov 4, 2022

Commits 4 & 5: Fixed the issues raised in comments

  • Added method size() to queueu_record_list_t
  • Fixed the code to not treat copy like copyin
  • Removed modified parts which were not in compliance with GPUFORT concept (e.g., gpufortrt_devide_t)

Commit 3: Updated python part

  • Added a few lines that convert acc library routines to gpufort

Commit 2: additional examples to test acc rtlib

  • Added some simple examples for testing purposes

Commit 1: resolved minor errors in forked repo

  • Defined gpufortrt_device_t dev_type = "AMD" by default
  • Didnt know about the correct value that should be passed to function setup in file queue_record_list_t.cpp. Should it be -1?
  • Copy and copyin look identical (at the moment). I should further read the specification and implement copy
  • Commented out (for now) problematic functions like variants of function gpufortrt_get_property.

@monoatamd monoatamd requested a review from domcharrier November 4, 2022 12:59
runtime/gpufortrt/include/gpufortrt_api.h Outdated Show resolved Hide resolved
runtime/gpufortrt/include/gpufortrt_api.h Outdated Show resolved Hide resolved
runtime/gpufortrt/include/gpufortrt_api.h Outdated Show resolved Hide resolved
runtime/gpufortrt/include/gpufortrt_api.h Outdated Show resolved Hide resolved
runtime/gpufortrt/src/gpufortrt_api.cpp Outdated Show resolved Hide resolved
@domcharrier domcharrier marked this pull request as draft November 4, 2022 13:10
HIP_CHECK(hipMemGetInfo(&free, &total))
return free;
case gpufortrt_property_free_memory:
size_t freeMem;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the renaming? Usage of camelCase does not fit with rest of code base. I am planning to adopt the LLVM programming style eventually but not for the time being.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In switch case, the scope of variables is the switch not the cases: we get errors about the redefinition of variables free and total.

Either we have to put the definition of free and total in the switch (which looks weird) or we need to define new variables. How about free_mem and total_mem?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in commit 59d4293

Copy link
Collaborator

@domcharrier domcharrier Nov 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two more things:

  1. Probably makes sense in this case to declare the inout arguments outside
    of the case to prevent naming conflicts.
  2. Alternatively, you can use '{' and '}' like this
    case <label> { ... } ( don't forget to break )
    to have a local scope for variables within the different cases where needed.

Copy link
Author

@monoatamd monoatamd Nov 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took first option and fixed it in commit a69f450

@@ -629,6 +629,7 @@ def is_end_statement_(tokens, kind):
current_linemap["statements"]):
try:
expand_statement_functions_(current_statement)
original_statement = current_statement["body"]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't accept frontend changes now as I am currently changing it on a separate development branch.
I suggest to keep a local copy and not include to this PR.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please feel free to reject the changes. I have local copies.

@@ -1942,6 +1942,10 @@ def is_declaration(tokens):
def is_blank_line(statement):
return not len(statement.strip())

def is_acc_rtlib(statement, modern_fortran):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't accept frontend changes now as I am currently changing it on a separate development branch.
I suggest to keep a local copy and not include to this PR.

Copy link
Author

@monoatamd monoatamd Nov 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please feel free to reject the changes. I have local copies

!
end function
end interface
if(c_associated(gpufortrt_is_present_c_impl(c_loc(data_arg),int(bytes,kind=c_size_t)))) then
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can make an assignment out of this.
Please further do not use the camelCase for the result arg,
breaks with the OpenACC look and feel.
You could also just write

logical :: gpufortrt_is_present

...

gpufort_is_present = c_associated(...)

and get rid of the result(...) alltogether.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in commit 59d4293

@@ -292,4 +292,31 @@ function gpufortrt_use_device_c_impl(hostptr,condition,if_present) &
resultptr = gpufortrt_use_device_c_impl(hostptr,opt_if_arg,opt_if_present_arg)
end function

function gpufortrt_is_present(data_arg, bytes) result(isOnDeviceMemory)
use iso_c_binding
Copy link
Collaborator

@domcharrier domcharrier Nov 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If not intended (in this case it shouldn't), try not to forget the implicit none after the use statements and before the first declaration / line of code.
Otherwise, Fortran's default implicit naming rules are active, which based on the first letter of an identifier
will assign a default integer or real type to undeclared variables.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in commit 59d4293

interface
function gpufortrt_is_present_c_impl(data_arg, bytes) &
bind(c,name="gpufortrt_present") result(isOnDeviceMemory)
use iso_c_binding
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also use implicit none here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we use implicit none once at the module scope? This way we don't need to repeat it for every interface/function/subroutine

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in commit 59d4293

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could do that (works both for program and module) but it would not apply to the interfaces; see
https://stackoverflow.com/a/24337734
Which can cause confusion.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok then, I avoid putting it at module scope to keep it clear

!> \param[in] if_present Only return device pointer if one could be found for the host pointer.
!> otherwise host pointer is returned. Defaults to '.false.'.
!> \note Returns a c_null_ptr if the host pointer is invalid, i.e. not C associated.
subroutine gpufortrt_use_device(resultptr, hostptr,sizes,lbounds,if_arg,if_present)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm working on use_device, as it doesn't work at the moment:

I didnt get the logic in previous implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants